Repository navigation
Fix resumable job progress across runtimes - #1521
Conversation
bgentry
changed the title
74e3217 to
d761ae5
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| metadata[CURSOR_KEY] = null; | ||
| const metadata: JsonObject = {}; | ||
| if (this.#completedStep !== null) metadata[STEP_KEY] = this.#completedStep; | ||
| if (this.#cursorsDirty) { |
There was a problem hiding this comment.
Bot-generated: Nonblocking follow-up; this also reproduces on the base commit.
A successful checkpoint leaves its cursor in #cursors while clearing #cursorsDirty. If a nested cursor step subsequently calls setCursor(), this branch writes the entire map again. For example: checkpoint parent: 1, record child: 7 in a nested step, then roll back the parent's transaction and fail. Deferred metadata still contains both cursors, so the retry can skip the parent's rolled-back work. The same state/dirty-flag pattern exists in Go and Rust.
I reproduced the JavaScript case on both this PR and its base. This is an unusual combination with a real correctness consequence, so it's worth tracking separately with a regression test. I wouldn't block merging this PR on it.
| // The database owns this checkpoint, including rollback of the caller's | ||
| // transaction. Only progress recorded after this write belongs in deferred | ||
| // attempt metadata; replaying it could persist rolled-back application work. | ||
| state.CompletedStep = "" |
There was a problem hiding this comment.
Bot-generated: Nonblocking documentation clarification; pre-existing behavior.
Clearing CompletedStep here is undone when the surrounding ResumableStep callback returns successfully. If the caller's transaction encloses the step and subsequently rolls back or fails to commit, a worker error still causes the final update to persist that completed-step marker.
Committing the transaction before the step callback returns successfully is the natural safe usage pattern. Documenting that requirement here and on the equivalent Rust/JavaScript APIs seems sufficient for this PR. I reproduced the equivalent JavaScript case on the base commit too; this isn't a new regression or a reason to delay merging.
|
@bgentry Posted some LLM-generated feedback here too. Would normally take a bit more time on it, but let's bias towards getting all this stuff in. |
d761ae5 to
e55befb
Compare
e55befb to
88abef3
Compare
Keep transactional checkpoints out of deferred attempt metadata after successful writes so a later failure cannot persist progress whose application transaction rolls back. Preserve earlier deferred progress when the checkpoint write itself fails. Follow Ruby's rule that the transaction owns a successful write, allowing earlier unpersisted steps to run again after rollback. Persist cursor changes independently of completed steps so a failed first step can resume from its cursor. Retain Rust and JavaScript progress on snooze and other incomplete outcomes, including in worker helpers. Cover rollback, commit followed by failure, failed writes, subsequent cursor changes, first-step retries, snooze, and cooperative shutdown with regressions and shared conformance scenarios. Require checkpoint transactions to commit before their step callbacks return successfully, and document propagation of transaction and commit errors. A successful callback otherwise records completion independently of a later rollback by an enclosing transaction. Keep the SQLite rescue assertion's normal busy timeout so concurrent worker writes do not fail it spuriously. Allow its result deadline to outlast the database lock timeout. Update the locked `ctutils` patch release to resolve its constant-time comparison advisory.
88abef3 to
f45a4df
Compare
Resumable jobs save completed steps and cursors so retries can skip finished work. This PR fixes three bugs that made retries skip work that never committed or repeat work already done.
1. Retries can skip work that rolled back. River normally saves progress from memory when an attempt ends. Transactional helpers write a checkpoint immediately, inside the worker’s transaction. That checkpoint must commit or roll back with the application changes.
In Go,
ResumableSetStepTxandResumableSetStepCursorTxalso added the checkpoint to memory before their database write succeeded. Their Rust equivalents and JavaScript’sresumable.checkpointhad the same bug. A worker could update a customer row’s address and save the customer’s ID as a cursor in the job’s metadata, all in one transaction. If the transaction rolled back, River still saved the cursor with the failed attempt. The retry then skipped that customer even though the address update never committed. A failed checkpoint write could cause the same bug.The fix has to work without knowing how the transaction ends: River cannot see whether a caller-owned transaction later commits or rolls back. If the attempt's final update saved the checkpoint, it could restore one that rolled back. If it saved only the earlier progress, it could overwrite one that committed. So after a successful checkpoint write, the final update saves neither. It saves only progress recorded after that write. A failed checkpoint write leaves pending progress unchanged.
The cost is that a rollback can make the retry repeat earlier steps whose progress existed only in memory. Ruby already works this way. Suppose
preparecompletes beforeimportcheckpoints cursor1, with no previously saved progress:prepare; startsimportwithout a cursorprepare; resumesimportat cursor1prepare; startsimportwithout a cursor2. A cursor in the first step is lost. A cursor records progress within an unfinished step. Go, Rust, and JavaScript saved cursors only when at least one step had completed. Java's equivalent fix has since landed on master.
If the first step processed items 1–7, recorded cursor
7, and failed, the retry received no cursor and repeated those items.River now saves cursors independently of step completion. The retry receives cursor
7so it can continue with item 8. This also preserves cursors on snooze and cooperative shutdown.3. Snoozing drops progress in Rust and JavaScript. Snoozing ends the attempt and schedules another run. Both runtimes treated snooze as a successful worker return, but saved progress only on error. A worker that completed
downloadand snoozed beforetransformtherefore downloaded everything again.Both runtimes now save progress unless the job completed, matching Go and Ruby. Their worker test helpers follow the same rule.
Shared conformance cases cover these behaviors, including failed checkpoint writes and progress recorded after a checkpoint.
Go, Rust, and JavaScript checkpoint updates still match only on job ID. An execution that continues after timing out can still overwrite a newer execution's progress. This is not addressed here.